Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added consistent footer #1

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

Wassaf-Shahzad
Copy link

@Wassaf-Shahzad Wassaf-Shahzad commented Sep 29, 2021

Pre-Flight checklistScreenshot 2021-10-06 at 2 46 59 PM

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop width screenshots
  • Testing
    • Changes have been manually tested

What are the relevant tickets?

Closes mitodl/mitxonline#223

What's this PR do?

Updates the frontend-app-footer to MITX specifications

How should this be manually tested?

This PR can verified by the following way

  • Add required environment variables in env of parent application
  • Verify footer meets specifications

Screenshots (if appropriate)

Screenshot 2021-09-30 at 3 32 32 PM

Updated Image

Screenshot 2021-10-06 at 2 46 59 PM

@Wassaf-Shahzad Wassaf-Shahzad added the Work in progress PR currently in process label Sep 29, 2021
@Wassaf-Shahzad
Copy link
Author

Work in progress look of footer

Screenshot 2021-09-28 at 11 24 58 PM

@Wassaf-Shahzad Wassaf-Shahzad force-pushed the wassaf/add-consistant-footer-mitxonline branch from ed76ff2 to 76bb7c8 Compare September 30, 2021 10:36
@briangrossman
Copy link

The design is looking good. A couple questions/comments:

  • Is it possible to remove the MIT icon from the left side of the footer?
  • Can you please add -'s between the footer items

Here's the mockup
footer

@Wassaf-Shahzad
Copy link
Author

The design is looking good. A couple questions/comments:

  • Is it possible to remove the MIT icon from the left side of the footer?
  • Can you please add -'s between the footer items
  • Made the logo configurable
  • Added "-" between links
  • Linked Contact to configurable email.

Screenshot 2021-10-01 at 4 47 42 PM

Screenshot 2021-10-01 at 4 48 32 PM

There is this issue when there is a line break

Screenshot 2021-10-01 at 4 59 39 PM

@Wassaf-Shahzad Wassaf-Shahzad added Needs Review and removed Work in progress PR currently in process labels Oct 1, 2021
@Wassaf-Shahzad Wassaf-Shahzad force-pushed the wassaf/add-consistant-footer-mitxonline branch 2 times, most recently from 3f7acda to 2180fe4 Compare October 1, 2021 12:23
@pdpinch
Copy link
Member

pdpinch commented Oct 5, 2021

We don't need that "Choose Language" form. Will it still appear even when edx-platform is configured for just one language?

@pdpinch
Copy link
Member

pdpinch commented Oct 5, 2021

@briangrossman question for you -- if the logos and link targets are configurable, can we use this for xPRO (and Residential MITx) as well?

.env.development Outdated
@@ -14,6 +14,16 @@ SEGMENT_KEY=null
SITE_NAME=Open edX
USER_INFO_COOKIE_NAME=edx-user-info
LOGO_URL=https://edx-cdn.org/v3/default/logo.svg
LOGO_TRADEMARK_URL=https://edx-cdn.org/v3/default/logo-trademark.svg
LOGO_TRADEMARK_URL=https://rc.mitxonline.mit.edu/static/images/mit-logo.jpg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about committing project-specific values to this dev file.

Copy link
Author

@Wassaf-Shahzad Wassaf-Shahzad Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about committing project-specific values to this dev file.

These values are actually picked up from the parent MFE and we can leave it blank as well but I will update it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed project related default envs.

@Wassaf-Shahzad
Copy link
Author

We don't need that "Choose Language" form. Will it still appear even when edx-platform is configured for just one language?

No, it depends if you want to display it or not. I activated to test responsiveness and if link would display correctly with it On.

@briangrossman
Copy link

@briangrossman question for you -- if the logos and link targets are configurable, can we use this for xPRO (and Residential MITx) as well?

@pdpinch I'm not as familiar with Residential, but this footer is very similar to xPRO. xPRO also has a Support Center link, which wouldn't work in MITx Online yet. If the code would skip elements that didn't have the link target set, I'd say this works for xPRO and MITx Online at a minimum.

@pdpinch
Copy link
Member

pdpinch commented Oct 5, 2021 via email

@Wassaf-Shahzad
Copy link
Author

We should have a support center for mitxonline in a few weeks, so it would be worth adding that. However, there is no support center for Residential, so the link should be conditional on whether the environment variable is set.

Added conditional support center link with configurable url and text.

@briangrossman
Copy link

@Wassaf-Shahzad
Regarding the screenshot above where you mention the issue when there is a line break, take a look at the footer in xPRO. It's set up with a break point so that it's either:

  • all on one line
  • fully stacked with one item per line

I think that approach would work fine in this case. Particularly with the addition of another item (Support center)

@Wassaf-Shahzad
Copy link
Author

Wassaf-Shahzad commented Oct 6, 2021

@Wassaf-Shahzad Regarding the screenshot above where you mention the issue when there is a line break, take a look at the footer in xPRO. It's set up with a break point so that it's either:

  • all on one line
  • fully stacked with one item per line

I think that approach would work fine in this case. Particularly with the addition of another item (Support center)

@briangrossman
The code Identical to MITxOnline however I was under the impression that we could break the single row into multiple rows (as shown in the image above) before finally going to one item per line strategy as the screen size is reduced but as we only have two strategies (described above) I have updated the breakpoint and attached an updated image

Screenshot 2021-10-06 at 7 45 55 PM

@Wassaf-Shahzad Wassaf-Shahzad added Work in progress PR currently in process Needs Review and removed Needs Review Work in progress PR currently in process labels Oct 7, 2021
Copy link

@asadiqbal08 asadiqbal08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wassaf-Shahzad related to this image that you attached. If you look at the footer level then English and apply drop list are not seems me aligned.

https://user-images.githubusercontent.com/87968618/135615361-f3bded77-c092-4c47-9d05-2203cd2c6399.png

}
<div class="copyright-col">
<div class="text-gray-500 small">
{process.env.TRADEMARK_TEXT}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is make sense, to add a additional condition here to check for the text to be available or not. I mean, if TRADEMARK_TEXT is not set in env then it add a empty div here

</div>
<div>
<ul class="footer-sub-nav">
<li><a href={process.env.ABOUT_US_URL}>About Us</a></li>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, these links should not be rendered if they are not setup.

@Wassaf-Shahzad
Copy link
Author

@Wassaf-Shahzad related to this image that you attached. If you look at the footer level then English and apply drop list are not seems me aligned.

https://user-images.githubusercontent.com/87968618/135615361-f3bded77-c092-4c47-9d05-2203cd2c6399.png

The language selector is not needed in this component. I enabled it in the above image to test responsiveness.

@asadiqbal08
Copy link

asadiqbal08 commented Oct 14, 2021

@Wassaf-Shahzad if we are not using the Language Selector then I think the existing logic should be refactor or hidden by some environment variables. It is hidden by default ? How do you enable it ?

<Footer
          onLanguageSelected={() => {}}
          supportedLanguages={[
            { label: 'English', value: 'en' },
            { label: 'Español', value: 'es' },
          ]}
        />

@asadiqbal08 asadiqbal08 removed their assignment Oct 14, 2021
Copy link

@asadiqbal08 asadiqbal08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok had discussion 1-1 with Wassaf about the Language Selector that is hidden by default.
👍

@Wassaf-Shahzad
Copy link
Author

supportedLanguages={[
        { label: 'English', value: 'en' },
        { label: 'Español', value: 'es' },
      ]}
    />

The language selector is enabled by passing a prop to the footer.It is disabled by default (as no prop is passed) and we can refactor it to use and environment variable but that depends if it is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

open edX: implement a consistent, shared footer
4 participants